@llvm/pr-subscribers-mlir

Author: None (arthurqiu)

diff --git a/mlir/cmake/modules/IRDLToCpp.cmake b/mlir/cmake/modules/IRDLToCpp.cmake
index 8470ccdf55166..2457773ef2565 100644
--- a/mlir/cmake/modules/IRDLToCpp.cmake
+++ b/mlir/cmake/modules/IRDLToCpp.cmake
@@ -5,7 +5,7 @@ function(add_irdl_to_cpp_target target irdl_file)
 
     # The command output depends on the executable to ensure IRDL sources are properly rebuilt
     # if the tool changes.
-    DEPENDS ${MLIR_IRDL_TO_CPP_EXE} ${CMAKE_CURRENT_SOURCE_DIR}/${irdl_file}
+    DEPENDS ${MLIR_IRDL_TO_CPP_TARGET} ${CMAKE_CURRENT_SOURCE_DIR}/${irdl_file}
     COMMENT "Building ${irdl_file}..."
   )
   add_custom_target(${target} DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${irdl_file}.cpp.inc)

@Moxinilian request for review. Thanks!

Have you checked the comment above? Can you make sure that it still correctly rebuilds the sources if the tool changes? If this is not the case you may want to depend on both the target and the executable.

Not sure if I miss anything, but I think IRDL source should still be rebuilt if it depends on MLIR_IRDL_TO_CPP_TARGET.

https://cmake.org/cmake/help/latest/command/add_custom_command.html

If the argument is the name of a target (created by the add_custom_target(), add_executable(), or add_library() command) a target-level dependency is created to make sure the target is built before any target using this custom command. Additionally, if the target is an executable or library, a file-level dependency is created to cause the custom command to re-run whenever the target is recompiled.

Could you please try? This is what I did the first time and it did not work.

In parallel I am asking for help on the Discord server. It would be nice if there was a way to test the build system for this property automatically somehow, but this seems tricky to set up.

I'm not 100% sure on what's going on here but generally these tools are referred to by *_EXE instead of the target for cross-compile (specifically the NATIVE stuff you're seeing) because they're precursors that need to run on the host. So eg mlir-linalg-ods-yaml-gen is built for host instead of target because it generates source at build time of mlir-opt and etc:

Looking at mlir-irdl-to-cpp/CMakeLists.txt I see someone tried to maybe do the same thing?? but didn't quite nail it? Then again I'm not sure what the intent for this tool is. But certainly if you want add_irdl_to_cpp_target to be run in the service of e.g. building mlir-opt then you need a host tool and so you wouldn't want to DEPEND on the target.

Hmm looking at the custom command for add_linalg_ods_yaml_gen I see it does depend on MLIR_LINALG_ODS_YAML_GEN_TARGET:

Okay in the case maybe this is kosher - although that CMake for the tool itself is still fishy but 🤷.

This tool behaves exactly like mlir-tblgen (in fact it serves the same purpose). So it should be built on the host platform, not on the target platform. I also understood it should be referenced via EXE as it is obtained from the NATIVE build. So the problem seems to be that in your environment @arthurqiu it is not able to find how to build it? I had tried before that it works while cross-compiling and it did. Are you sure your environment is correctly configured? In any case it should work no matter what, so I’d like to know what failed in your set up.

Also maybe I just misremember what I did at the time and this works for incremental rebuilds if the tool change. I can verify it does if you don’t have time but I don’t have much time either right now so it may take a bit.

I have reproduced the problem I was referring to with this patch applied. But the good news is that I also tried having the target depend on both $MLIR_IRDL_TO_CPP_EXE and $MLIR_IRDL_TO_CPP_TARGET and it seems to not reproduce the problem (because the EXE dependency still retriggers the build). If I understand your problem correctly, it should also solve yours. Please let me know if that works.

I have reproduced the problem I was referring to with this patch applied. But the good news is that I also tried having the target depend on both $MLIR_IRDL_TO_CPP_EXE and $MLIR_IRDL_TO_CPP_TARGET and it seems to not reproduce the problem (because the EXE dependency still retriggers the build). If I understand your problem correctly, it should also solve yours. Please let me know if that works.

It works for me. And this explains why we need both:

Well it seems like I can’t accept now that I merged it. Anyway, thanks for the fix!